Use templates for common geometric constants #7558

rubiefawn · 2024-10-23T00:30:48Z

lmms_constants.h has several common constants such as PI and E, with variants of each for float, double, and long double. Many of these are currently unused in the repo but are of potential use. This change replaces LD_PI, D_PI, and F_PI with lmms::numbers::pi, and so on, in the style of C++20's <numbers>.

Currently, the constants are set using literal values, but once C++20 is fully ready these can be changed to their std::numbers::* equivalents instead.

i re-added the wrong constant ffs

messmerd · 2024-10-23T05:52:46Z

You should use inline constexpr since the variables are in a header, though maybe it would be best to wait for C++20 (#7554) and GCC 10 where we'll get the <numbers> header.

Updated to account for `<numbers>` and C++20: - Marked all `lmms_constants.h` constants with an exact equivalent in `<numbers>` as deprecated - Removed all `lmms_constants.h` constants where no variant is currently in use - Using `inline constexpr` - Using `std::floating_point` concept instead of `typename`

rubiefawn · 2024-10-23T19:47:56Z

Updated to account for <numbers> and C++20:

Marked all lmms_constants.h constants with an exact equivalent in <numbers> as deprecated
Removed all lmms_constants.h constants where no variant is currently in use
Using inline constexpr
Using std::floating_point concept instead of typename

messmerd · 2024-10-23T19:58:10Z

This won't work. I suggested waiting for C++20 and the compiler upgrade since we are in the process of doing that now.

Currently we are using C++17 and GCC 9.3 for Linux and MinGW builds, so we can't use the <numbers> header or concepts yet.

If you wanted, maybe you could convert our mathematical constants to use the same names as those in <numbers>, but in the lmms::numbers namespace. Then once we upgrade to C++20, switching to <numbers> will be as easy as swapping out lmms::numbers::pi for std::numbers::pi. That may be quite a bit of work for you though, so it's up to you.

EDIT: The constants should probably be in a new lmms::numbers namespace to avoid collisions. It would basically just be a reimplementation of the parts of the numbers header that we use.

rubiefawn · 2024-10-24T01:09:33Z

I'm aware that we don't have C++20 ready yet. I made the suggested changes (and more) in advance under the assumption that this PR was now dependent on #7554, based off of your suggestion. If this is not the case I can change it back, I'm good with whatever.

My original intention with this PR was small in scope; I just wanted to clean up lmms_constants.h. That being said, I love your suggestion for lmms::numbers, and if updating all usage of the constants is on the table, I'd love to explore that.

messmerd · 2024-10-24T04:17:35Z

Ah gotcha. Sorry for the miscommunication. The C++20 support could happen tomorrow if people could review the PR, but upgrading our MinGW compiler and dependencies will be tricky and take some time.

I think the lmms::numbers idea would be good to implement. Function scope usages could use using namespace numbers; so that it doesn't need to be typed out fully as the clunkier numbers::pi or numbers::pi_v<float> in places where that would look better.
I'm not sure how big a task it would be, but it would probably be fairly easy with vscode or some other program to just search through src/, include/, and plugins/ and replace all.

sakertooth · 2024-11-01T18:20:46Z

If I understand @messmerd correctly, I agree with him. We should wait for C++20, remove lmms_constants.h, and then just use the numeric library provided by the standard. The more code we can cut down and remove, I see as a win.

rubiefawn · 2024-11-02T06:05:10Z

Just to clarify, there are several useful constants in lmms_constants.h that have no overlap with C++20 <numbers>. This PR isn't about removing that header entirely, it's just to clean it up. I agree with the "less is more" sentiment though.

On that note, I noticed panning_constants.h and panning.h and am wondering if those can be merged into one file (or even just moved to lmms_constants.h). Those could be considered further candidates for constant cleanup.

The current plan is to wait for #7554 before I make further changes. Input is welcome in the meantime!

sakertooth · 2024-11-23T16:52:00Z

#7554 is here, so you can try and make the switch to the numbers header @rubiefawn (we have partial C++20 support, but I think we should have the numbers header at least).

On that note, I noticed panning_constants.h and panning.h and am wondering if those can be merged into one file (or even just moved to lmms_constants.h). Those could be considered further candidates for constant cleanup.

It's up to you really (or anyone else that wants to give it a go). I personally think it could go multiple ways with no apparent downside.

messmerd · 2024-11-23T19:01:20Z

@sakertooth Nope, like most C++20 features, we're waiting for GCC 10.

See: https://en.cppreference.com/w/cpp/compiler_support/20
(Mathematical constants P0631R8)

sakertooth · 2024-11-23T21:05:59Z

Hard to classify it as an upgrade then, but I guess it's a step in the right direction..

messmerd · 2024-11-23T22:47:54Z

I'm using both class type NTTP and designated initializers in the pin connector now, so it's not nothing. Once I'm done with the pin connector, I'd like to upgrade to GCC 11 on Linux and MinGW 13 for the cross-compiled Windows builds. Then we'll have nearly all C++20 features including <numbers>.

rubiefawn · 2024-11-28T06:01:43Z

This is the second time I've gotten duplicated commit history after pushing. Not sure if GitHub just doesn't like rebase or if it's a skill issue (likely the latter). How do I avoid this in the future?

Rossmaxx · 2024-11-28T06:14:53Z

Don't worry, we do squash on merge.

Rossmaxx · 2024-11-28T06:16:58Z

Wait, seems like lots of unnecessary diffs (from previously merged prs)

Moves the four constants in panning_constants.h into panning.h, then removes panning.h.

Rossmaxx

Some thoughts which came across when I looked at the diff.

include/BasicFilters.h

include/lmms_constants.h

Rossmaxx · 2024-11-28T07:36:04Z

plugins/Eq/EqCurve.cpp

-
-	const double w = 2 * LD_PI * freq / SR ;
-	const double PHI = pow( sin( w / 2 ), 2 ) * 4;
+	const double w = sin(numbers::pi * freq / Engine::audioEngine()->outputSampleRate());


You sure about this?

// original const int SR = Engine::audioEngine()->outputSampleRate(); const double w = 2 * LD_PI * freq / SR ; const double PHI = pow( sin( w / 2 ), 2 ) * 4; // update LD_PI to new numbers::pi const int SR = Engine::audioEngine()->outputSampleRate(); const double w = 2 * numbers::pi * freq / SR ; const double PHI = pow( sin( w / 2 ), 2 ) * 4; // SR is used only once, just use outputSampleRate() directly instead const double w = 2 * numbers::pi * freq / Engine::audioEngine()->outputSampleRate(); const double PHI = pow( sin( w / 2 ), 2 ) * 4; // w is used only once and is divided by 2, so the multiply by 2 cancels out const double w = numbers::pi * freq / Engine::audioEngine()->outputSampleRate(); const double PHI = pow( sin( w ), 2 ) * 4; // pow(x, 2) == x * x, simplify into multiplication expression const double w = numbers::pi * freq / Engine::audioEngine()->outputSampleRate(); const double PHI = sin(w) * sin(w) * 4; // no need to call sin(w) twice, just assign the sin()'d value to w in the first place const double w = sin(numbers::pi * freq / Engine::audioEngine()->outputSampleRate()); const double PHI = w * w * 4;

I should have renamed w to make it clear it's not intended to have the exact same value as the previous code. Is that what you meant?

There seems to be a change in mathematical calculation. That's what I meant.

Nvm did some pen paper testing and the math checks out.

Also there's one thing you can do, make outputSampleRate into a member variable with initialisation in the constructor to avoid calling the function every time. It gets a bit lengthy.

rubiefawn added 4 commits October 22, 2024 18:21

add templates for common geometric constants

c8592ba

oops missed one

fdc56b4

LD_2PI -> LD_PI

ae0dfc8

i re-added the wrong constant ffs

CamelCase names and also verify compilation without -DLMMS_MINIMAL

447a157

rubiefawn marked this pull request as draft October 24, 2024 00:43

rubiefawn added 2 commits November 28, 2024 00:01

add lmms::numbers namespace

3a61877

Remove panning_constants.h

0794170

Moves the four constants in panning_constants.h into panning.h, then removes panning.h.

rubiefawn force-pushed the template-literals branch from dff4d78 to 0794170 Compare November 28, 2024 07:03

rubiefawn marked this pull request as ready for review November 28, 2024 07:27

Rossmaxx reviewed Nov 28, 2024

View reviewed changes

Use std::exp(n) instead of powf(numbers::e, n)

d96ff66

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use templates for common geometric constants #7558

Use templates for common geometric constants #7558

rubiefawn commented Oct 23, 2024 •

edited

Loading

messmerd commented Oct 23, 2024

rubiefawn commented Oct 23, 2024

messmerd commented Oct 23, 2024 •

edited

Loading

rubiefawn commented Oct 24, 2024

messmerd commented Oct 24, 2024

sakertooth commented Nov 1, 2024

rubiefawn commented Nov 2, 2024

sakertooth commented Nov 23, 2024

messmerd commented Nov 23, 2024

sakertooth commented Nov 23, 2024

messmerd commented Nov 23, 2024

rubiefawn commented Nov 28, 2024

Rossmaxx commented Nov 28, 2024

Rossmaxx commented Nov 28, 2024

Rossmaxx left a comment

Rossmaxx Nov 28, 2024

rubiefawn Nov 29, 2024

Rossmaxx Nov 29, 2024

Rossmaxx Nov 29, 2024

Rossmaxx Nov 29, 2024

Use templates for common geometric constants #7558

Are you sure you want to change the base?

Use templates for common geometric constants #7558

Conversation

rubiefawn commented Oct 23, 2024 • edited Loading

messmerd commented Oct 23, 2024

rubiefawn commented Oct 23, 2024

messmerd commented Oct 23, 2024 • edited Loading

rubiefawn commented Oct 24, 2024

messmerd commented Oct 24, 2024

sakertooth commented Nov 1, 2024

rubiefawn commented Nov 2, 2024

sakertooth commented Nov 23, 2024

messmerd commented Nov 23, 2024

sakertooth commented Nov 23, 2024

messmerd commented Nov 23, 2024

rubiefawn commented Nov 28, 2024

Rossmaxx commented Nov 28, 2024

Rossmaxx commented Nov 28, 2024

Rossmaxx left a comment

Choose a reason for hiding this comment

Rossmaxx Nov 28, 2024

Choose a reason for hiding this comment

rubiefawn Nov 29, 2024

Choose a reason for hiding this comment

Rossmaxx Nov 29, 2024

Choose a reason for hiding this comment

Rossmaxx Nov 29, 2024

Choose a reason for hiding this comment

Rossmaxx Nov 29, 2024

Choose a reason for hiding this comment

rubiefawn commented Oct 23, 2024 •

edited

Loading

messmerd commented Oct 23, 2024 •

edited

Loading